Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Generate unique UIDs and GIDs #663

Merged
merged 14 commits into from
Jan 17, 2025
Merged

Generate unique UIDs and GIDs #663

merged 14 commits into from
Jan 17, 2025

Conversation

adombeck
Copy link
Contributor

@adombeck adombeck commented Dec 2, 2024

These changes have already partly been reviewed in https://github.com/canonical/authd-private/pull/9.

Closes #509
UDENG-5416
UDENG-4352

@adombeck adombeck force-pushed the 509-random-unique-ids branch 3 times, most recently from cb2ba1b to 8e15caf Compare December 3, 2024 00:05
internal/users/localgroups/getpwent_c.go Outdated Show resolved Hide resolved
internal/users/localgroups/getpwent_c.go Outdated Show resolved Hide resolved
internal/testsdetection/testsdetection.go Outdated Show resolved Hide resolved
internal/users/localgroups/getpwent_c.go Outdated Show resolved Hide resolved
internal/users/manager.go Outdated Show resolved Hide resolved
internal/users/manager.go Outdated Show resolved Hide resolved
internal/users/localentries/getpwent_c.go Show resolved Hide resolved
internal/users/localentries/getpwent_test.go Outdated Show resolved Hide resolved
internal/users/manager.go Outdated Show resolved Hide resolved
internal/users/manager.go Outdated Show resolved Hide resolved
internal/users/manager.go Outdated Show resolved Hide resolved
internal/users/manager.go Outdated Show resolved Hide resolved
internal/users/manager.go Outdated Show resolved Hide resolved
internal/users/manager.go Outdated Show resolved Hide resolved
internal/users/manager.go Outdated Show resolved Hide resolved
internal/users/manager.go Outdated Show resolved Hide resolved
internal/users/manager.go Outdated Show resolved Hide resolved
internal/users/manager.go Outdated Show resolved Hide resolved
internal/users/manager.go Outdated Show resolved Hide resolved
internal/users/manager.go Outdated Show resolved Hide resolved
@adombeck adombeck force-pushed the 509-random-unique-ids branch from 8e15caf to 745d7c6 Compare December 5, 2024 23:18
@adombeck
Copy link
Contributor Author

adombeck commented Dec 5, 2024

rebased on main and resolved conflicts

@adombeck adombeck force-pushed the 509-random-unique-ids branch 5 times, most recently from 12c61bb to 75fe88c Compare December 20, 2024 15:03
@adombeck adombeck marked this pull request as ready for review December 21, 2024 16:20
@adombeck adombeck requested a review from a team as a code owner December 21, 2024 16:20
@adombeck
Copy link
Contributor Author

I extracted the new code to handle temporary users and groups into a separate package and added some more tests.

@adombeck adombeck requested a review from 3v1n0 January 6, 2025 13:53
@adombeck adombeck force-pushed the 509-random-unique-ids branch 2 times, most recently from 0eb85a5 to e186eca Compare January 8, 2025 15:54
Copy link
Collaborator

@3v1n0 3v1n0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we're almost there, some further nits and suggestions.

I was wondering, since there are some common code paths in groups and users temporary entries registration, would be possible to do some logic deduplication, or is it not possible?

internal/users/manager_test.go Outdated Show resolved Hide resolved
internal/services/nss/nss.go Outdated Show resolved Hide resolved
internal/users/tempentries/groups.go Outdated Show resolved Hide resolved
internal/users/tempentries/groups.go Outdated Show resolved Hide resolved
internal/users/tempentries/groups.go Outdated Show resolved Hide resolved
internal/users/localentries/getgrent_c.go Outdated Show resolved Hide resolved
internal/users/localentries/getgrent_c.go Outdated Show resolved Hide resolved
internal/users/localentries/getgrent_c.go Outdated Show resolved Hide resolved
internal/users/localentries/getpwent_c.go Outdated Show resolved Hide resolved
internal/users/localentries/getpwent_c.go Outdated Show resolved Hide resolved
@adombeck adombeck requested a review from 3v1n0 January 10, 2025 09:10
internal/users/localentries/errno.go Outdated Show resolved Hide resolved
internal/users/localentries/getgrent_c.go Outdated Show resolved Hide resolved
internal/users/localentries/getpwent_c.go Outdated Show resolved Hide resolved
@adombeck adombeck force-pushed the 509-random-unique-ids branch from e0ac675 to 3a2e9b0 Compare January 13, 2025 16:13
@adombeck adombeck requested a review from 3v1n0 January 13, 2025 16:16
@adombeck adombeck force-pushed the 509-random-unique-ids branch from 3a2e9b0 to 58cc388 Compare January 15, 2025 17:12
@adombeck
Copy link
Contributor Author

Rebased on main

adombeck and others added 14 commits January 16, 2025 21:22
Should have been removed by 4fd03a4.
Combine TestUserByID and TestUserByName, TestGroupByID and
TestGroupByName, because the test bodies were identical (except for the
called function).
* Remove obsolete tests: They were testing behavior which has changed.
* Fix tests by using either predictable UIDs or replacing UIDs in golden
  files with placeholders (normalizing)
* Limit number of pre-auth user records to avoid denial of service by
  sending many SSH requests for unique users to fill up the UID range.
* Remove field UID from users.UserInfo struct because it's not set in
  internal/brokers/broker.go anymore and there is no need to expose this
  field.
* Rename localgroups package to localentries because it now also
  provides functions for passwd entries.
* Check for error when calling getpwent or getgrent
We only need to unset it once we've used it in a loop, while in the
other cases we'd expect it being set or unset by the function that has
been called
@3v1n0 3v1n0 force-pushed the 509-random-unique-ids branch from b664389 to eaeae48 Compare January 16, 2025 21:23
@adombeck
Copy link
Contributor Author

@3v1n0 Anything missing for your approval?

@3v1n0
Copy link
Collaborator

3v1n0 commented Jan 17, 2025

@3v1n0 Anything missing for your approval?

No, a part me remembering of hitting the green button :-D

Copy link
Collaborator

@3v1n0 3v1n0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, thanks for spending time in getting authd the less racy we can!

@3v1n0 3v1n0 merged commit aba1e3c into main Jan 17, 2025
11 checks passed
@3v1n0 3v1n0 deleted the 509-random-unique-ids branch January 17, 2025 12:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Issue: GID for group already in use by a different group
2 participants